fix(skills): reject an unknown flag instead of installing it as the skill source - #227
Merged
ralyodio merged 1 commit intoAug 3, 2026
Conversation
…kill source `skill install` took the first unconsumed token as the source, so a flag it does not know became the source itself and the URL the user typed was dropped: `skill install -s user https://github.com/o/r.git` installs a skill called `s` from a source of `-s`, and never looks at `user` or the URL again. The source is spliced verbatim into each engine's native argv, so gemini gets `skills install -s --scope user` and Claude gets `git clone --depth 1 -s <skills-dir>/s`, where `-s` is git's own `--shared` and makes git read the destination as the repository. `mcp` already rejects a stray flag for this exact reason; this applies the same guard to the same kind of splice.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
skill installtakes the first unconsumed token as the source:Nothing checks whether that token is a flag. So a flag
skilldoes not know becomes the source, and the URL the user actually typed is dropped without a word:A skill named
s, from a source of-s.userand the URL are never looked at again.The source is spliced verbatim into each engine's native argv, which is where it gets sharp:
-sis git's own--shared, so git stops treating the last path as the destination and reads it as the repository:A typo lands in the same place —
skill install --nmae my-skill <url>installs a skill callednmaefrom a source of--nmaeand silently discards the URL.The fix
Reject a source that still starts with
-after parsing, and say which flagskill installdoes take. This is the guardmcpalready applies to its own spec (src/integrations.mjs, "A token still starting with-at this point was never consumed as a flag…", added in #164) — same failure mode, same kind of argv splice, so the same rule rather than a new one.The
./-sescape hatch is real, not decorative —git clone --depth 1 ./-s destclones fine, verified.Verification
Reproduced before touching code, with fake
git/geminibins onPATHlogging their argv and a sandboxedHOME— the argv lines above are that log verbatim. Re-ran the same three cases after the fix: both bad cases now exit 1, and the control (skill install <url>) produces byte-identical argv to before.Tests: new
test/skill-stray-flag.test.mjs, 11 tests. 5 pin the bug and fail before this change; 6 are controls that pass in both directions (a normal URL still installs,--namestill names, a./pathsource is untouched, the pre-existing--name requires a valueguard still wins, no-source still prints usage, an unknown verb still says unknown verb). They driveskillCommandthrough therun/installedSetseam it already exposes, so no new export and no signature change.Suite: 905 → 916 tests, 727 passing, 0 failing.
Scope
Deliberately not fixed here, so the diff stays one thing:
skill install <url> extra junkignoresextraandjunk. That is a real second gap, but it is a separate behaviour change with its own compatibility question, and it is not what makes the case above dangerous.skill install <url> -s user) is still ignored rather than rejected.mcpdeliberately lets trailing positionals through as the command's own args;skillhas no args concept, so the right answer there is arguable and worth its own change.